ESQL: Fix `mv_expand` inconsistent column order #129745

kanoshiou · 2025-06-20T07:53:56Z

The new attribute generated by mv_expand should remain in the original field positions. However, when ProjectAwayColumns is executed top-down and encounters an MvExpandExec with the child plan of ExchangeExec, the inserted Project may not respect that order.

Closes #129000

# Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

elasticsearchmachine · 2025-07-11T17:13:42Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

alex-spies

Super nice find, thank you @kanoshiou !

I think the fix would work, but I'd like to attempt a fix that will also work for other plan nodes that we add in the future and might work similarly to MV_EXPAND; see my suggestion below.

x-pack/plugin/esql/qa/testFixtures/src/main/resources/mv_expand.csv-spec

.../src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/ProjectAwayColumns.java

alex-spies · 2025-07-15T16:22:11Z

Thanks again for your contribution @kanoshiou !

Do you mind if we push to your PR in order to try and augment the fix as described here?

kanoshiou · 2025-07-16T06:20:31Z

Thanks for the review, @alex-spie!

I've updated the branch based on your comments and please feel free to push changes as needed!

alex-spies

Great, @kanoshiou , thanks for the iteration! This is very close. I only have a final set of remarks; once the remarks in ProjectAwayColumns.java are addressed, this LGTM.

alex-spies · 2025-07-16T07:44:50Z

.../src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/ProjectAwayColumns.java

+                                requiredAttrBuilder.remove(attribute);
+                            }
+                        }
+                        output.addAll(requiredAttrBuilder.build());


Hm, adding the remaining required attributes is not wrong, but the requiredAttrBuilder should really be empty after removing all the attributes that we added to output, because all the required attributes have to be in the fragment's outputSet - otherwise, we're constructing an invalid plan.

Rather than removing attributes from requiredAttrBuilder and performing this addAll here, I think we can just assert that the size of output and the size of requiredAttrBuilder are the same. (Although the PlanConsistencyChecker will likely also catch this.)

all the required attributes have to be in the fragment's outputSet

I gave it some thought, and it does make sense.

assert that the size of output and the size of requiredAttrBuilder are the same

I have a question: my understanding is that requiredAttrBuilder should be a subset of output, meaning its size should be less than or equal to that of output. Why, then, are they the same size here? If their sizes are the same, then why bother maintaining requiredAttrBuilder? Wouldn't it be ok to just use output for execution?

Sorry, I confused outputSet with the new array list output.

Well, I was wrong. There's a bug in the planning of remote enrich that can make it so that the output of the fragment doesn't contain an attribute, but it's actually physically there and the projection needs to include it, or a downstream TopN won't know what attribute to sort on. This is what happened in #118531.

@kanoshiou , I'll re-instate your initial approach (but still using output rather than outputSet) and I'll add a comment to the problem.

Thanks @alex-spies!

.../src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/ProjectAwayColumns.java

...in/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/PhysicalPlanOptimizerTests.java

alex-spies · 2025-07-16T08:02:28Z

...in/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/PhysicalPlanOptimizerTests.java


        project = as(exchange.child(), ProjectExec.class);
-        assertThat(names(project.projections()), contains("abbrev", "name", "location", "country", "city"));
+        assertThat(names(project.projections()), contains("abbrev", "city", "country", "location", "name"));


Nice. The only reason these queries produced correct output was because there was a Project at the top that made the attribute order here irrelevant; but actually, we had changed it locally, and this shows that ProjectAwayColumns was never fully correct to begin with.

alex-spies · 2025-07-16T08:10:22Z

@elasticmachine test this please

alex-spies · 2025-07-16T09:37:39Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/mv_expand.csv-spec

+
+testMvExpandInconsistentColumnOrder3
+required_capability: fix_mv_expand_inconsistent_column_order
+from message_types,languages_lookup_non_unique_key 


The multi cluster tests don't like the languages_lookup_non_unique_key index because it's a lookup index, and it's treated differently; the languages index is used in an enrich policy, and the multicluster tests also treat such indices differently. Finally, the CSV tests don't like having more than 1 index.

I'm going to push an update that replaces these tests by ones that use indices that our csv tests and multi cluster tests don't treat especially :)

Indices used in enrich policies, as lookup indices, or multiple indices in the FROM pattern are all treated differently by different test suites.

alex-spies · 2025-07-16T09:41:40Z

@elasticmachine test this please

kanoshiou · 2025-07-16T10:46:29Z

Thanks @alex-spies. I've updated the branch and please review when you have a moment.

alex-spies

This is great, thanks @kanoshiou !

alex-spies · 2025-07-16T12:56:56Z

@elasticmachine test this please

Keep all the attributes from requiredAttrBuilder in the projection, even when they're not in the fragment's output due to a bug.

alex-spies · 2025-07-16T15:28:29Z

@elasticmachine test this please

kanoshiou · 2025-07-16T15:36:32Z

Oh, I forgot to remove this line. It seems the CI test has not started yet. I'll update the branch shortly.

kanoshiou · 2025-07-16T15:42:11Z

Please start a new round of test @alex-spies, thank you!

alex-spies · 2025-07-16T17:02:34Z

@elasticmachine test this please

alex-spies · 2025-07-17T08:23:20Z

Here we go! Thanks again @kanoshiou !

The new attribute generated by MV_EXPAND should remain in the original position. The projection added by ProjectAwayColumns does not respect the original order of attributes. Make ProjectAwayColumns respect the order of attributes to fix this.

The new attribute generated by MV_EXPAND should remain in the original position. The projection added by ProjectAwayColumns does not respect the original order of attributes. Make ProjectAwayColumns respect the order of attributes to fix this. (cherry picked from commit ac0c508) # Conflicts: # x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/generative/GenerativeRestTest.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/ProjectAwayColumns.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/PhysicalPlanOptimizerTests.java

The new attribute generated by MV_EXPAND should remain in the original position. The projection added by ProjectAwayColumns does not respect the original order of attributes. Make ProjectAwayColumns respect the order of attributes to fix this. Co-authored-by: kanoshiou <[email protected]>

The new attribute generated by MV_EXPAND should remain in the original position. The projection added by ProjectAwayColumns does not respect the original order of attributes. Make ProjectAwayColumns respect the order of attributes to fix this.

…1428) * ESQL: Fix inconsistent column order in MV_EXPAND (#129745) The new attribute generated by MV_EXPAND should remain in the original position. The projection added by ProjectAwayColumns does not respect the original order of attributes. Make ProjectAwayColumns respect the order of attributes to fix this. (cherry picked from commit ac0c508) # Conflicts: # x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/generative/GenerativeRestTest.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/ProjectAwayColumns.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/PhysicalPlanOptimizerTests.java * Adapt approach for missing AttributeSetBuilder Builder is only available on main/9.x, so we have to correctly use the immutable AttributeSets directly. --------- Co-authored-by: kanoshiou <[email protected]>

The new attribute generated by MV_EXPAND should remain in the original position. The projection added by ProjectAwayColumns does not respect the original order of attributes. Make ProjectAwayColumns respect the order of attributes to fix this. (cherry picked from commit ac0c508) # Conflicts: # x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/generative/GenerativeRestTest.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

alex-spies · 2025-07-17T13:56:46Z

💔 Some backports could not be created

Status	Branch	Result
✅	9.0
✅	8.19
❌	8.18	Could not create pull request: fetch failed

Manual backport

To create the backport manually run:

backport --pr 129745

Questions ?

Please refer to the Backport tool documentation

The new attribute generated by MV_EXPAND should remain in the original position. The projection added by ProjectAwayColumns does not respect the original order of attributes. Make ProjectAwayColumns respect the order of attributes to fix this. (cherry picked from commit ac0c508) # Conflicts: # x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/AttributeSet.java # x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/generative/GenerativeRestTest.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/ProjectAwayColumns.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/PhysicalPlanOptimizerTests.java

The new attribute generated by MV_EXPAND should remain in the original position. The projection added by ProjectAwayColumns does not respect the original order of attributes. Make ProjectAwayColumns respect the order of attributes to fix this. (cherry picked from commit ac0c508) # Conflicts: # x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/generative/GenerativeRestTest.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java Co-authored-by: kanoshiou <[email protected]>

The new attribute generated by MV_EXPAND should remain in the original position. The projection added by ProjectAwayColumns does not respect the original order of attributes. Make ProjectAwayColumns respect the order of attributes to fix this. (cherry picked from commit ac0c508) # Conflicts: # x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/AttributeSet.java # x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/generative/GenerativeRestTest.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/ProjectAwayColumns.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/PhysicalPlanOptimizerTests.java Co-authored-by: kanoshiou <[email protected]>

Fix mv_expand inconsistent column order

834b2b7

elasticsearchmachine added needs:triage Requires assignment of a team area label v9.1.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Jun 20, 2025

Update docs/changelog/129745.yaml

9cd191e

elasticsearchmachine added v9.2.0 and removed v9.1.0 labels Jun 26, 2025

kanoshiou and others added 2 commits July 2, 2025 09:53

Merge branch 'refs/heads/main' into fix-inconsistent-column-order

478062a

# Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

Merge branch 'main' into fix-inconsistent-column-order

0cb21e0

PeteGillinElastic added :Analytics/ES|QL AKA ESQL and removed needs:triage Requires assignment of a team area label labels Jul 11, 2025

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jul 11, 2025

alex-spies requested changes Jul 15, 2025

View reviewed changes

alex-spies self-assigned this Jul 15, 2025

alex-spies added >bug v8.19.0 v9.1.0 v9.0.5 v8.18.5 auto-backport Automatically create backport pull requests when merged labels Jul 15, 2025

kanoshiou added 2 commits July 16, 2025 13:16

Merge branch 'refs/heads/main' into fix-inconsistent-column-order

09af1ed

Respect the order of the attributes

dad35c7

alex-spies reviewed Jul 16, 2025

View reviewed changes

Use simpler indices in csv tests

0906f11

Indices used in enrich policies, as lookup indices, or multiple indices in the FROM pattern are all treated differently by different test suites.

alex-spies approved these changes Jul 16, 2025

View reviewed changes

Merge branch 'main' into fix-inconsistent-column-order

531559b

alex-spies mentioned this pull request Jul 16, 2025

ESQL: Improve Enrich handling in mapper/remote enrich planning #115897

Closed

alex-spies added 2 commits July 16, 2025 17:26

Revert to kanoshiou's initial approach

9ff4935

Keep all the attributes from requiredAttrBuilder in the projection, even when they're not in the fragment's output due to a bug.

Merge branch 'main' into fix-inconsistent-column-order

1ec5ac1

Remove 129000

66ed7d8

Merge branch 'main' into fix-inconsistent-column-order

c39cc44

alex-spies merged commit ac0c508 into elastic:main Jul 17, 2025
34 checks passed

alex-spies mentioned this pull request Jul 17, 2025

[9.1] ESQL: Fix inconsistent column order in MV_EXPAND (#129745) #131413

Merged

elasticsearchmachine added the backport pending label Jul 17, 2025

alex-spies mentioned this pull request Jul 17, 2025

[9.0] ESQL: Fix inconsistent column order in MV_EXPAND (#129745) #131428

Merged

alex-spies mentioned this pull request Jul 17, 2025

[8.19] ESQL: Fix inconsistent column order in MV_EXPAND (#129745) #131447

Merged

alex-spies mentioned this pull request Jul 17, 2025

[8.18] ESQL: Fix inconsistent column order in MV_EXPAND (#129745) #131448

Merged

ESQL: Fix mv_expand inconsistent column order #129745

ESQL: Fix mv_expand inconsistent column order #129745

Uh oh!

Conversation

kanoshiou commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Jul 11, 2025

Uh oh!

alex-spies left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alex-spies commented Jul 15, 2025

Uh oh!

kanoshiou commented Jul 16, 2025

Uh oh!

alex-spies left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alex-spies Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

kanoshiou Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

kanoshiou Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

alex-spies Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

kanoshiou Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alex-spies Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

alex-spies commented Jul 16, 2025

Uh oh!

alex-spies Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

alex-spies commented Jul 16, 2025

Uh oh!

kanoshiou commented Jul 16, 2025

Uh oh!

alex-spies left a comment

Choose a reason for hiding this comment

Uh oh!

alex-spies commented Jul 16, 2025

Uh oh!

alex-spies commented Jul 16, 2025

Uh oh!

kanoshiou commented Jul 16, 2025

Uh oh!

kanoshiou commented Jul 16, 2025

Uh oh!

alex-spies commented Jul 16, 2025

Uh oh!

Uh oh!

alex-spies commented Jul 17, 2025

Uh oh!

alex-spies commented Jul 17, 2025

💔 Some backports could not be created

Manual backport

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

ESQL: Fix `mv_expand` inconsistent column order #129745

ESQL: Fix `mv_expand` inconsistent column order #129745

kanoshiou commented Jun 20, 2025 •

edited

Loading

alex-spies left a comment •

edited

Loading

alex-spies left a comment •

edited

Loading